Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing protobuf generation and InstrumentationScope #665

Merged
merged 5 commits into from
Apr 27, 2022
Merged

Fixing protobuf generation and InstrumentationScope #665

merged 5 commits into from
Apr 27, 2022

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Apr 25, 2022

switching over to using opentelemetry-proto's own mechanism for generating protobuf interfaces, and
updating interfaces to latest version which works with our grpc exporter (v0.14.0). A separate issue + PR will
be required to work out why grpc exporting doesn't work with v0.15.0+

fixes: #660

brettmc added 3 commits April 25, 2022 22:28
switching over to using opentelemetry-proto's own mechanism for generating protobuf interfaces, and
updating interfaces to latest version
this is the latest version that seems to work with out grpc exporter, further
investigation required for 0.15.0+
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #665 (20fa81d) into main (9138d78) will increase coverage by 0.00%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #665   +/-   ##
=========================================
  Coverage     85.25%   85.26%           
  Complexity     1153     1153           
=========================================
  Files           128      128           
  Lines          2788     2789    +1     
=========================================
+ Hits           2377     2378    +1     
  Misses          411      411           
Flag Coverage Δ
7.4 85.21% <94.28%> (+<0.01%) ⬆️
8.0 85.26% <94.28%> (+<0.01%) ⬆️
8.1 85.26% <94.28%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SDK/Common/Instrumentation/KeyGenerator.php 100.00% <ø> (ø)
src/SDK/Trace/SpanBuilder.php 0.00% <0.00%> (ø)
src/Contrib/Jaeger/SpanConverter.php 96.47% <100.00%> (ø)
src/Contrib/Otlp/SpanConverter.php 100.00% <100.00%> (ø)
src/Contrib/OtlpGrpc/Exporter.php 90.16% <100.00%> (+0.16%) ⬆️
src/Contrib/Zipkin/SpanConverter.php 97.22% <100.00%> (ø)
...DK/Common/Instrumentation/InstrumentationScope.php 100.00% <100.00%> (ø)
src/SDK/Trace/ImmutableSpan.php 100.00% <100.00%> (ø)
src/SDK/Trace/Span.php 93.70% <100.00%> (ø)
src/SDK/Trace/Tracer.php 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9138d78...20fa81d. Read the comment docs.

rm -R ./$REPO_DIR
git clone https://github.com/open-telemetry/$REPO_DIR
cd "${DESTINATION_DIR}" || exit
rm -rf ./${REPO_DIR}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want a check for something here. If ${REPO_DIR} ends up being empty or being inadvertently set some other way......

Copy link
Collaborator Author

@brettmc brettmc Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's set a couple of lines up, so I think this is safe. I've confirmed that it's portable enough to be run from / or /scripts and still work... And, it works if ${REPO_DIR} is empty (which is usually is)

echo "$TAG"
echo "Generating protobuf files for version ${TAG} ..."
make gen-php
rm -rf ${GPBMETA_DIR} ${OTEL_DIR}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same in re: rm -rf


echo "Cleaning up..."
rm -rf ./${REPO_DIR}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto in re: rm -rf

@brettmc brettmc marked this pull request as draft April 25, 2022 23:21
@brettmc
Copy link
Collaborator Author

brettmc commented Apr 25, 2022

setting this to draft, since I now think I can get it working with 0.16 - the issue looks to be the renaming of InstrumentationLibrary to Scope

brettmc added 2 commits April 27, 2022 17:21
…rotobuf

InstrumentationLibrary is now deprecated in favour of InstrumentationScope. Making this change
resolves issues with grpc+protobuf exports being ignored by the otel collector, so we now
can update the proto interfaces to latest.
@brettmc brettmc changed the title Fixing protobuf generation Fixing protobuf generation and InstrumentationScope Apr 27, 2022
@brettmc brettmc marked this pull request as ready for review April 27, 2022 07:37
@bobstrecansky bobstrecansky merged commit 4e32645 into open-telemetry:main Apr 27, 2022
@brettmc brettmc deleted the proto-gen branch October 25, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protobuf generation broken
2 participants